Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #951 +/- ##
=======================================
Coverage 96.20% 96.20%
=======================================
Files 218 218
Lines 14373 14373
=======================================
Hits 13828 13828
Misses 545 545 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…NCH env, set names to human typable names
… pyproject.toml, update flake.nix
…downloaded aswell
…age, update check examples to pre built correct version
…ix shell dependencies
mhoff
left a comment
There was a problem hiding this comment.
Many thanks for the work! I did a first review pass
| @@ -0,0 +1 @@ | |||
| use flake | |||
There was a problem hiding this comment.
This is only required for nix build, right? I am a bit hesitant to add this new file solely for builds and I would assume that it would also be loaded for all other operations (like running logprep etc.)
There was a problem hiding this comment.
this is for direnv, if you have direnv installed and allow the directory, it auto loads the nix flake. I definitly need it, I previously did not commit it, but I have seen it on nearly every big open source project, so I thought it would be best practice to commit aswell. This by itself does nothing
| with: | ||
| version: latest | ||
| - name: Set up Nix | ||
| uses: cachix/install-nix-action@v31 |
There was a problem hiding this comment.
Commit SHA. I assume you did not do this as the PR is not finalized yet, but I still wanted to add the comment as a reminder not to forget for future reviews
| - name: Run Compose setup | ||
| working-directory: examples/compose | ||
| run: docker compose up -d | ||
| run: PYTHON_VERSION="${{ matrix.python-version }}" docker compose up -d |
There was a problem hiding this comment.
This was a bug right? ... nice catch
There was a problem hiding this comment.
Yeah it probably was, I think before we actually always just used py3.11 for the checks it seems. I just changed this, because there now is no image called logprep:py3.11 when running in py3.12 matrix as we dont build it directly in the docker file
| working-directory: examples/compose | ||
| run: docker compose build --build-arg PYTHON_VERSION=${{ matrix.python-version }} | ||
| run: | | ||
| export PY_VERSION="${{ matrix.python-version }}" |
There was a problem hiding this comment.
Why not use the same wording as below (PYTHON_VERSION)? Does nix build use PY_VERSION internally? If not, let's simply use PYTHON_VERSION and put in under env together with COMPOSE_PROFILES. This way (as we are reusing compose-env in "Run Compose setup" we have it all in one go.
There was a problem hiding this comment.
This has to do with the nameing of the docker file, .#docker.python311 is the current name for building 3.11, using .#docker.3.11 would then be something different as .#docker.311, thats why we export it as a variable and then do some Bash magic on it to transform 3.11 to python311, without the dot
There was a problem hiding this comment.
Ahh no now I get what you mean, yeah ill change that. I was a little confused
| * install requirements with `uv sync --frozen --extra doc` in project root | ||
|
|
||
| ## nix version | ||
| * nix develop |
There was a problem hiding this comment.
I don't get what this means. Maybe still a placeholder. Needs more context because people are typically not familiar with nix
Edit: installation.rst makes it more clear, but still
…ntainer version scheme, delete unused Dockerfile
Co-authored-by: Michael Hoff <9436725+mhoff@users.noreply.github.com>
Co-authored-by: Michael Hoff <9436725+mhoff@users.noreply.github.com>
Description
Assignee
Documentation
Code Quality
How did you verify that the changes work in practice?
Reviewer
The rendered docs for this PR can be found here.